CancellableFuture: Some cleanups and few more tests #1846
Conversation
3v1n0
force-pushed
the
cancellation-things
branch
from
05ee47b to
829fa5e
Compare
API break is fine, just can't backport that then so maybe do it in separate commits on top of this. |
3v1n0
force-pushed
the
cancellation-things
branch
3 times, most recently
from
9438b6c to
3a85305
Compare
There's no need to use a channel, we can just block on the spawning future
Since CancellableFuture can be used group and manage async operations that support cancellation, add tests to ensure that this is the case
Handle SIGINT via the unix signal future and cancellation via the CancellableFuture
It's just a placeholder, but using cancellable is confusing, since it's an actual type that GTask supports.
Instead of connecting to the signal only when polling starts, and do potential disconnections and re-connections, do it just once so that there is no risk that a signal may happen during this phase, and disconnect only after the future has been dropped.
It would be nicer to just return the actual glib::Error, but that would imply breaking the API sadly, so let's just craft it manually
3v1n0
force-pushed
the
cancellation-things
branch
from
3a85305 to
0da285e
Compare
| cancellable: Cancellable, | ||
| connection_id: Option<CancelledHandlerId>, | ||
|
|
||
| waker: std::sync::Arc<std::sync::Mutex<Option<std::task::Waker>>>, |
There was a problem hiding this comment.
i'd avoid fullspec, Arc and Mutex can be imported to improve readability (no other Arc and Mutex, so there is no ambiguity)
Same comment in other part of this diff
| if self.cancellable.is_cancelled() { | ||
| // XXX: Whenever we want to break the API, we should return here only | ||
| // self.cancellable.set_error_if_cancelled() value. | ||
| return std::task::Poll::Ready(Err(Cancelled)); |
There was a problem hiding this comment.
just Poll::Ready, useless full-spec
| fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| if self.is_cancelled() { | ||
| return Poll::Ready(Err(Cancelled)); | ||
| if self.cancellable.is_cancelled() { |
There was a problem hiding this comment.
why this change, equivalent to before
| #[strong] | ||
| waker, | ||
| move |_| { | ||
| if let Some(waker) = waker.lock().unwrap().take() { |
There was a problem hiding this comment.
personal opinion, but I generally never use unwrap in PROD code. Rather .expect("msg") where "msg" is a brief explanation of the logical invariant which holds the condition of why you think that never happens.
Feel free to ignore this comment tho, maybe on lock is quite self-explanatory
|
|
||
| let mut this = self.as_mut().project(); | ||
| let mut waker = self.waker.lock().unwrap(); | ||
| if waker.is_none() { |
There was a problem hiding this comment.
This changes the logical behavior.
In rust there is no guarantee the a future is polled with the same waker. The waker can change and here we are ignoring the update. The previous code was intentional made because of this.
There was a problem hiding this comment.
I see. Well we should update the waker then rather than the connection, since the waker is what we can lock on and we've control
There was a problem hiding this comment.
I have two thoughts on this:
- The polling pattern could be improved to avoid unnecessary locking. Something like:
match .poll() {
Ready() => { .. }
Pending => {
self.waker = waker;
}
}This approach would be cleaner as it only sets the waker when actually needed (on Pending), and eliminates the need for explicit drops, probably. Anyway that should be the typical pattern.
- As a side consideration: Have you considered the thread-safety implications? Specifically, there could be race conditions if we wake the waker immediately before checking poll. While this may not be an issue given GLib's threading model, it's something I've encountered when working with low-level async Rust code that requires retry loops
| let mut this = self.as_mut().project(); | ||
| let mut waker = self.waker.lock().unwrap(); | ||
| if waker.is_none() { | ||
| *waker = Some(cx.waker().clone()); |
There was a problem hiding this comment.
Nit: Arc::clone(&t) is more explicit about why and how the global counter is used
| } | ||
| } | ||
|
|
||
| impl std::error::Error for Cancelled {} |
There was a problem hiding this comment.
I would move the XXX:... comment of yours here, mentioning that maybe we can get rid of a specific error and just wrap the "cancel" error into a glib::Error. With API break of coure
3v1n0
changed the title
3 participants
Coming from #46 and handled some of the initial changes I was suggesting.
Sadly main improvements would require an API break, so I did just some minimal lifting.